Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Generate doc stubs for transplanted methods #2176

Closed
wants to merge 5 commits into from
Closed

Conversation

jbarnoud
Copy link
Contributor

The transplant_stub.py script introspects the groups and topology
attributes to write files in documentation_pages/core that contain the
documentation for the transplanted methods.

For the stubs to be picked up by sphinx, the docstring of the class to
document must contain

.. include:: XXX.txt

where "XXX" is the name of the class.

A stub contains a table of the methods, their short descriptions, and
what topology attribute they require.

Fixes #1845

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@jbarnoud
Copy link
Contributor Author

It looks like this:

screenshot from 2019-01-12 19-35-02


screenshot from 2019-01-12 19-35-32

@codecov
Copy link

codecov bot commented Jan 12, 2019

Codecov Report

Merging #2176 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2176   +/-   ##
========================================
  Coverage    89.13%   89.13%           
========================================
  Files          143      143           
  Lines        17475    17475           
  Branches      2713     2713           
========================================
  Hits         15576    15576           
  Misses        1290     1290           
  Partials       609      609
Impacted Files Coverage Δ
package/MDAnalysis/core/groups.py 97.39% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24972e6...fb8f421. Read the comment docs.

The `transplant_stub.py` script introspects the groups and topology
attributes to write files in `documentation_pages/core` that contain the
documentation for the transplanted methods.

For the stubs to be picked up by sphinx, the docstring of the class to
document must contain

    .. include:: XXX.txt

where "XXX" is the name of the class.

A stub contains a table of the methods, their short descriptions, and
what topology attribute they require.

Fixes #1845
The syntax is not supported in python 3.5
Locally, sphinx complains about citation duplicates. Though, travis
complains about missing citations refering about the same citation.
@zemanj
Copy link
Member

zemanj commented Jan 23, 2019

That looks like a pretty pretty hacky but probably versatile solution.

The only thing I don't like about it is that the function signatures are the ones of the original methods and not the transplanted ones. This will most likely confuse users.
For example, fragments ends up being a property of AtomGroup, so it should appear without the parentheses. Likewise, methods such as center_of_mass() should not show up with group as their first argument.

A rather ugly and certainly more tedious solution is to manually insert the transplanted methods in the .. autoclass:: sections of the respective module.

Example for AtomGroup:

"""
.. autoclass:: AtomGroup
   :members:
   :inherited-members:

   .. autodata::
      MDAnalysis.core.topologyattrs.Bonds.fragments
      :annotation:

   .. autofunction::
      MDAnalysis.core.topologyattrs.Masses.center_of_mass(pbc=None, \
      compound='group')
"""

The empty :annotation: section is necessary to suppress the nonsensical bogus annotation which is otherwise auto-generated by sphinx. Using .. autoattribute:: instead of .. autodata:: unfortunately doesn't yield the expected result.

@jbarnoud
Copy link
Contributor Author

Yes. It is a bit hacky... I was not expecting to have to manually call napoleon, nor did I expect to have to introspect the signatures.

I should be able to treat the properties in a different way as I already have to detect them to grab their docstring.

It is not sustainable to add the methods manually to the doc; the doc will lag behind very rapidly as we forget we have to do the manual operation. Though, it is possible to have the 'include' in the 'autoclass' rather than in the class docstrings. Would there be any benefit?

Currently, the problem I have is that I do not know what to do with the references. If I keep the citations as is, sphinx complains about duplicates. If I remove them, then sphinx complains about missing citations. The only solution I can see is to introspect the modules doc to build a list of the citations, so I can remove only the reference that are already defined; though, I'd rather avoid doing that kind of book keeping.

@orbeckst
Copy link
Member

orbeckst commented Apr 3, 2019

Currently, the problem I have is that I do not know what to do with the references. If I keep the citations as is, sphinx complains about duplicates. If I remove them, then sphinx complains about missing citations. The only solution I can see is to introspect the modules doc to build a list of the citations, so I can remove only the reference that are already defined; though, I'd rather avoid doing that kind of book keeping.

Can you give an example of what the problematic case looks like?

@orbeckst
Copy link
Member

orbeckst commented Apr 3, 2019

Would it help if we decide that the only place where we have references is in documentation_pages/references.rst? We can add it to the Documentation Guide as a requirement. It would basically mean that within the docs you have to properly cite

  1. check if citations is in references.rst
  2. add if necessary
  3. use the citation key

Or perhaps we look into something like mcmtroffaes/sphinxcontrib-bibtex, at the cost of making doc writing a bit more convoluted.

@jbarnoud
Copy link
Contributor Author

Superseded by #2418.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs for many AtomGroup methods missing
3 participants